-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove dead attributes in application #673
Remove dead attributes in application #673
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## smartsim-refactor #673 +/- ##
=====================================================
+ Coverage 40.45% 42.72% +2.26%
=====================================================
Files 110 109 -1
Lines 7326 6980 -346
=====================================================
+ Hits 2964 2982 +18
+ Misses 4362 3998 -364
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Just some minor clean up here and there!!
I want to discuss some of those lingering TODOs with the group before I sign off and we merge anything, but otherwise this is looking like a much more refined model to me! Should be ready to go once we get consensus on those last few items!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks great! Just minor questions/comments.
smartsim/entity/application.py
Outdated
|
||
@property | ||
def exe_args(self) -> t.Sequence[str]: | ||
"""Return an immutable list of attached executable arguments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the returned value really immutable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At runtime, not technically, but the type hint here states that a sequence is return which implicitly promotes immutability. If we do drop the "immutable" wording from the docstring, I'm going to also recommend changing the return type to list[str]
or at the very least MutableSequence[str]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I guess I was thinking that _build_exe_args
returns a List
which is mutable so that part was out of sync.
Do we want to be specific and risk API breaks later by changing to list[str]
or generic to limit breaks later (i.e. MutableSequence[str]
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to mutablesequence[str]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more minor nits to consider while you sort out the conflict, but otherwise looks about ready to go to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last question, but nothing worth holding approval up over. LGTM! Thanks for all the hard work cleaning up this interface!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
ec7677a
into
CrayLabs:smartsim-refactor
No description provided.